Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VIM-1341 implement gx command #773

Closed
wants to merge 1 commit into from
Closed

Conversation

DwordPtr
Copy link

@DwordPtr DwordPtr commented Dec 5, 2023

If anything is not right here please let me know so I can fix it.
I absolutely love this plugin.

@DwordPtr DwordPtr changed the title implement gx command VIM-1341 implement gx command Dec 11, 2023
@AlexPl292
Copy link
Member

Hi there! Great, this is a long-awaited feature. I've added a couple of comments.

@DwordPtr
Copy link
Author

DwordPtr commented Dec 30, 2023

I just saw this.
I'll address them once I get back to my computer!

Edit: I'm having trouble finding the comments. I'll check back again in a bit.

@GhengS
Copy link

GhengS commented Jan 7, 2024

why 'gx' open in browser not work?

@AlexPl292
Copy link
Member

Hi @GhengS, This feature has not yet been merged with the IdeaVim plugin.

@DwordPtr
Copy link
Author

DwordPtr commented Jan 8, 2024

@AlexPl292 I can't find the comments.

Would you mind checking the submit review button? I'm guessing because I've made this mistake often with gh's ui.

operatorArguments: OperatorArguments,
): Boolean {
val wordUnderCursor = exactWordUnderCursor(editor);
logger.info("word: $wordUnderCursor")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging is always great, but we try not to log the raw text from user input. The problem is that this text may have sensitive information like passwords and we don't want to leak it into the log.
I see several cases here, it's better to clean them up.

): Boolean {
val wordUnderCursor = exactWordUnderCursor(editor);
logger.info("word: $wordUnderCursor")
if(!isValidUrl(wordUnderCursor)){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about the logic behind links search.
Generally, it looks valid to me, but IJ also has a feature to navigate to issues in Jira or youtrack by issue id. Like // Here is my issue: VIM-123. This, of course, won't be supported by this logic.
I actually found a fancy util in the platform: IssueNavigationConfiguration.getInstance(project).findIssueLinks(commentText). You can feed it a bunch of text and it will return resolved links (Including transformation from VIM-123 to https://youtrack.com/issues/VIM-123).
I think, it will be better to use this util. This, however, will make the code platform dependent, and we'll have two options here: move this GotoUrlAction out from vim-engine or make an abstraction via VimInjector. The second option would require to add a new function to VimInjector like VimApplication.resolveLinks and implement it via the mentioned util in IjVimInjector.

@AlexPl292
Copy link
Member

Oh, I'm really sorry, my eyes were ignoring the Pending status of the comments 🤦 . Do you see the comments now?

@DwordPtr
Copy link
Author

So I see your comments and have onced over how to address them.

I'll put some time into this when I'm not under deadline pressure at my day job.

@@ -23,7 +23,7 @@ repositories {
dependencies {
compileOnly("com.google.devtools.ksp:symbol-processing-api:1.9.21-1.0.15")
implementation("org.jetbrains.kotlinx:kotlinx-serialization-json-jvm:$kotlinxSerializationVersion") {
// kotlin stdlib is provided by IJ, so there is no need to include it into the distribution
// kotlin stdlib is provided by IJ, so there is no need to include it into the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental change?

@AlexPl292
Copy link
Member

Hi, this PR has been open for some time. Do you have plans to update it?

@DwordPtr
Copy link
Author

DwordPtr commented Jun 6, 2024

@AlexPl292
Sorry, I had a lot of personal life stuff happen over the last six months.

I would like to do this however it's gonna be a long time to implement this the proper way as Im new to text editors.

I think there's a couple options here.

  • use the older gx command as a holdover just to get vim parity
  • wait but merge another pr if someone gets to it before me.

Btw if you have some suggested reading for how to get up to speed quickly I'd gladly read it.

@AlexPl292
Copy link
Member

Hi, I see this PR is almost a year old. I'll close it, but feel free to reopen it in case you'll restart the work on it!

@AlexPl292 AlexPl292 closed this Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants